Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Windows: Remove use of deprecated function SHGetFolderPath and use SHGetKnownFolderPath instead #5862

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Feb 28, 2024

Fixes #5861

Based on the 6 years old comment at the top of OPAMW_SHGetFolderPath we might probably want to use the new API and don't want to spend the time supporting such an old system

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - apart from getting rid of youthfully exuberent (= daft) comments of mine from the source code, the fact SHGetKnownFolderPath handles allocation itself will automatically deal with long paths (which wasn't a thing when I wrote the original code; I just still harboured an unhealthy desire to keep old OSes working...!). I have a hunch that long paths might be what's gone wrong in the original issue (although it's surprising).

My instinct would be to have a single C stub taking a variant (more closely wrapping one API function), but actually I think your instinct is better in having separate functions! 🙂

We need to link opam with ole32 (for CoTaskMemFree) and uuid (for the FOLDERID_ "constants") - I'm pushing a commit which hopefully does that bit!

@kit-ty-kate kit-ty-kate force-pushed the windows-SHGetKnownFolderPath branch 5 times, most recently from c3d8903 to 8d630a6 Compare February 29, 2024 10:32
@kit-ty-kate kit-ty-kate added this to the 2.2.0~beta2 milestone Feb 29, 2024
@kit-ty-kate kit-ty-kate requested a review from dra27 February 29, 2024 11:09
kit-ty-kate and others added 2 commits March 4, 2024 13:27
…GetKnownFolderPath instead

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@kit-ty-kate kit-ty-kate force-pushed the windows-SHGetKnownFolderPath branch from 8d630a6 to 032e409 Compare March 4, 2024 13:28
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@kit-ty-kate kit-ty-kate merged commit 1eeaa74 into ocaml:master Mar 4, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the windows-SHGetKnownFolderPath branch March 4, 2024 16:00
@dra27 dra27 mentioned this pull request Jun 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opam for windows : make cold fails with OPAMW_SHGetFolderPath
3 participants